-
Notifications
You must be signed in to change notification settings - Fork 12
Reformat the gaussian conditional sampler #366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reformat the gaussian conditional sampler #366
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 99.43% 99.38% -0.06%
==========================================
Files 22 20 -2
Lines 1247 1140 -107
==========================================
- Hits 1240 1133 -107
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bthirion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for this refactoring. I have two relatively minor comments.
jpaillard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I agree that having both samplers implemented as classes makes sense.
jpaillard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it limits extra work to integrate the test in this PR rather than having to fix something in an upcoming one. I made a suggestion for the tests.
Co-authored-by: Joseph Paillard <[email protected]>
Co-authored-by: Joseph Paillard <[email protected]>
bthirion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is almost ready to merge
Co-authored-by: bthirion <[email protected]>
bthirion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a bit of fine tuning.
Co-authored-by: bthirion <[email protected]>
[skip tests]
bthirion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given #488, I have no further comments than @jpaillard's ones.
Co-authored-by: Joseph Paillard <[email protected]>
Co-authored-by: Joseph Paillard <[email protected]>
Co-authored-by: Joseph Paillard <[email protected]>
Co-authored-by: Joseph Paillard <[email protected]>
jpaillard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes
I reformat the Gaussian estimation of knockoff in the format of classes, as conditional sampling.
I moved all the conditional samplers into statistical_tool.